Review requested:

• @nodejs/gyp

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.33%. Comparing base (8c495c8) to head (50023d9).
⚠️ Report is 9 commits behind head on main.

@@           Coverage Diff            @@
##             main   #63575    +/-   ##
========================================
  Coverage   90.33%   90.33%            
========================================
  Files         730      730            
  Lines      234362   234464   +102     
  Branches    43908    43909     +1     
========================================
+ Hits       211708   211810   +102     
- Misses      14376    14378     +2     
+ Partials     8278     8276     -2     

@addaleax But this problem doesn't happen in the main thread. I had first thought about solving it in ObjectWrap as well, but that will not fix the case where addons use WeakCallback directly, and potentially other situations where a callback lingers. Also, as you mentioned, the ObjectWrap code has not been modified for many years and I would be hesitant to make any change there lest it break some other thing. It is exceptionally difficult to debug this type of bug because the code is gone so it doesn't appear in the stack trace. Per the comment that's there in the code, this was disabled for the main thread presumably because of certain issues, and the same ones will ostensibly happen if an add-on is only loaded in the worker thread, so IMO it would be good to align the main and worker thread behaviors. It also makes sense from a pure conceptual perspective that unloading should happen after all code has stopped running, and I'm not sure if there's some benefit for doing it earlier.

and potentially other situations where a callback lingers

That's part of the problem – this isn't specific to WeakCallbacks per se, and you can run into it with any addon that manually schedules callbacks.

Per the comment that's there in the code, this was disabled for the main thread presumably because of certain issues, and the same ones will ostensibly happen if an add-on is only loaded in the worker thread, so IMO it would be good to align the main and worker thread behaviors.

It's disabled in the main thread because legacy addons that are not aware of Environment cleanup hooks are expected to keep working as they are, that's all. This is also why we call out in our docs that addons need to explicitly opt into supporting worker threads: https://nodejs.org/api/addons.html#worker-support

Also, as you mentioned, the ObjectWrap code has not been modified for many years and I would be hesitant to make any change there lest it break some other thing.

That's not a good reason to keep it buggy tbh.

It also makes sense from a pure conceptual perspective that unloading should happen after all code has stopped running, and I'm not sure if there's some benefit for doing it earlier.

At the very least, there needs to be something that ensures that embedder-created Environment* instances still have their addons closed, which this PR currently breaks. But really, the Environment instance is conceptually what owns the addons, and if one has objects or callbacks that outlive the Enviroment, that's a bug (again, AddEnvironmentCleanupHook() is there for exactly this).

At the very least, there needs to be something that ensures that embedder-created Environment* instances still have their addons closed

There's an argument for not dlclosing at all. It is allowed to be a no-op after all. My understanding is that opening and closing libraries several times can cause issues. I could not find the original motivation for adding the close. On the one hand the dlopen happens inside Node.js, but in reality it's opened for the whole process, so it's not strictly tied to the Environment. Even if one were to add an environment cleanup hook, what if you need to do some asynchronous cleanup?

which this PR currently breaks

In the current behavior, without this PR, would that close code run if you created the embedder instance in the main thread?

It is allowed to be a no-op after all.

It is a no-op under specific circumstances, with legacy addons that have been written for older versions of Node.js that did not have cleanup logic in place.

My understanding is that opening and closing libraries several times can cause issues.

It can expose bugs in libraries, yes, but it's not inherently problematic.

On the one hand the dlopen happens inside Node.js, but in reality it's opened for the whole process, so it's not strictly tied to the Environment

Yeah, but the concept of ownership doesn't stop applying because it interacts with process-global state.

Even if one were to add an environment cleanup hook, what if you need to do some asynchronous cleanup?

Well ... you use the async environment cleanup hook version, right?

In the current behavior, without this PR, would that close code run if you created the embedder instance in the main thread?

Yeah, I have to correct myself here – currently we don't expose the is_main_thread() flag as something that is exposed to embedders. That's not great, but I guess right now embedders also don't get the benefits of cleanup.

It is a no-op under specific circumstances, with legacy addons that have been written for older versions of Node.js that did not have cleanup logic in place.

I meant on the OS level, dlclose can be a no-op per POSIX, as it is in musl.

Yeah, but the concept of ownership doesn't stop applying because it interacts with process-global state.

Yes, but shouldn't the process be the owner?

That's not great, but I guess right now embedders also don't get the benefits of cleanup.

In that case, would it make sense to now align the worker thread behavior with the main thread? At least until this is figured out.

Yeah, but the concept of ownership doesn't stop applying because it interacts with process-global state.

Yes, but shouldn't the process be the owner?

... no? That would be like saying that file descriptors are owned by processes and shouldn't be cleaned up by whichever entity opened them.

In that case, would it make sense to now align the worker thread behavior with the main thread?

In an ideal world, we'd have aligned these behaviors, yes – by also performing resource cleanup on the main thread. That hasn't been practical at any point so far, though.

At least until this is figured out.

I really don't think there's much that needs to be figured out here. If you'd prefer not to fix this bug where it occurs, I'm happy to open a PR myself (and, with your permission of course, include the test case from here).

... no? That would be like saying that file descriptors are owned by processes and shouldn't be cleaned up by whichever entity opened them.

Perhaps I worded that poorly. To match this analogy, the Node.js script is the one calling the open to receive a descriptor from the runtime, but the runtime is closing it (prematurely) while the user's program is still using it. With an OS file descriptor, if you open it but don't close it (as in the case of a Node.js script, which does not call close), then the OS will free that resource only once no references to it remain or the process terminates. This is not what is happening currently. The environment is not the one opening the addon, this is what is causing the dissonance for me, and that would explain my approach to the fix.

I really don't think there's much that needs to be figured out here. If you'd prefer not to fix this bug where it occurs, I'm happy to open a PR myself (and, with your permission of course, include the test case from here).

Please go ahead. Thank you for the feedback. It might be good to also re-open the linked issue until the fix has landed.